Skip to content

refactor: restructure mcp tools fetching with options object pattern #296

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Aug 11, 2025

Conversation

xierenyuan
Copy link
Contributor

@xierenyuan xierenyuan commented Aug 5, 2025

close #295

Copy link

changeset-bot bot commented Aug 5, 2025

🦋 Changeset detected

Latest commit: ce7f89a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages
Name Type
@openai/agents-core Patch
@openai/agents-openai Patch
@openai/agents-realtime Patch
@openai/agents Patch
@openai/agents-extensions Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@xierenyuan
Copy link
Contributor Author

Test results
sse
image
tool-filter
image
image

@vrtnis
Copy link
Contributor

vrtnis commented Aug 5, 2025

thanks for the options‑object updates, it’s definitely a nicer api going forward! I had some input on #164 that is referenced here. The original array‑only call to getAllMcpTools was removed in #164 (when tool‑filtering landed) and this PR replaces it with the options object, so existing code like getAllMcpTools(Array.from(servers.values())); i.e., 1‑arg form may fail.

As such, I'm thinking something along the lines of :

// overloads
export function getAllMcpTools(mcpServers: MCPServer[]): Promise<Tool[]>;
export function getAllMcpTools(opts: GetAllMcpToolsOptions): Promise<Tool[]>;

// implementation accepts MCPServer[] | opts
export async function getAllMcpTools(
  mcpServersOrOpts: MCPServer[] | GetAllMcpToolsOptions,
  runContext?: RunContext,
  agent?: Agent,
  convertSchemasToStrict = false,
) {
  const opts = Array.isArray(mcpServersOrOpts)
    ? { mcpServers: mcpServersOrOpts, runContext, agent, convertSchemasToStrict }
    : mcpServersOrOpts;
  // …existing logic…
}

Would you be open to restoring that legacy overload while keeping the new one? Happy to send a quick patch for this part. My quick thought here is that it would restore back‑compat, lets new callers use the options object (potentially the array form can be marked deprecated in a future PR)

@xierenyuan
Copy link
Contributor Author

xierenyuan commented Aug 6, 2025

thanks for the options‑object updates, it’s definitely a nicer api going forward! I had some input on #164 that is referenced here. The original array‑only call to getAllMcpTools was removed in #164 (when tool‑filtering landed) and this PR replaces it with the options object, so existing code like getAllMcpTools(Array.from(servers.values())); i.e., 1‑arg form may fail.

As such, I'm thinking something along the lines of :

// overloads
export function getAllMcpTools(mcpServers: MCPServer[]): Promise<Tool[]>;
export function getAllMcpTools(opts: GetAllMcpToolsOptions): Promise<Tool[]>;

// implementation accepts MCPServer[] | opts
export async function getAllMcpTools(
  mcpServersOrOpts: MCPServer[] | GetAllMcpToolsOptions,
  runContext?: RunContext,
  agent?: Agent,
  convertSchemasToStrict = false,
) {
  const opts = Array.isArray(mcpServersOrOpts)
    ? { mcpServers: mcpServersOrOpts, runContext, agent, convertSchemasToStrict }
    : mcpServersOrOpts;
  // …existing logic…
}

Would you be open to restoring that legacy overload while keeping the new one? Happy to send a quick patch for this part. My quick thought here is that it would restore back‑compat, lets new callers use the options object (potentially the array form can be marked deprecated in a future PR)

Hi, I've adjusted it. You see if it meets expectations

@seratch
Copy link
Member

seratch commented Aug 11, 2025

Thanks for sending this pull request. Overall, the changes look good to me, but could you look into the CI errors?

@xierenyuan
Copy link
Contributor Author

Thanks for sending this pull request. Overall, the changes look good to me, but could you look into the CI errors?

I'm done. If you need any more adjustments, please feel free to contact me.

Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you; LGTM

@seratch seratch merged commit c42a0a9 into openai:main Aug 11, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

getAllMcpTools introduced breaking changes
3 participants